All: Conflict notes will now populate a new field conflict_parent with the id of the conflict note. #5049

Ahmad45123 · 2021-06-03T19:55:50Z

As the title says. Pretty simple change to be used with https://github.com/joplin/plugin-conflict-resolution

…r note in conflict.

laurent22 · 2021-06-05T12:24:39Z

Should be "conflict_parent_id" for consistency with the rest of the code base.

Also please make sure that the is_conflict and your new field are in sync. If it's "1", your field should be set. If it's "0" it should be unset. I think there are some cases where it's not true.

roman-r-m · 2021-06-05T12:26:55Z

I think there are some cases where it's not true.

For instance when there's an unresolved conflict and you update Joplin to a version that includes this change.

Ahmad45123 · 2021-06-05T15:12:21Z

I think there are some cases where it's not true.

For instance when there's an unresolved conflict and you update Joplin to a version that includes this change.

Hmm how do I recover the original note ID though ? Unless I set it when a conflict occurs, I believe there's no way to know.

laurent22 · 2021-06-06T09:48:47Z

For instance when there's an unresolved conflict and you update Joplin to a version that includes this change.

Normally the is_conflict field is set to 1 only during sync so I'm not sure how updating Joplin would cause an issue?

roman-r-m · 2021-06-06T20:26:38Z

Normally the is_conflict field is set to 1 only during sync so I'm not sure how updating Joplin would cause an issue?

I was trying to say that when @Ahmad45123 is going to add logic that relies on this field he can't assume that each conflicted note will have it set.

Maybe there should be an option to set it manually?

laurent22 · 2021-06-06T21:09:50Z

I was trying to say that when @Ahmad45123 is going to add logic that relies on this field he can't assume that each conflicted note will have it set.

This field being set is what defines a conflicted note, so it would definitely be set. When a conflict is detected a new note is created with "is_conflict" set to 1. So it's not possible to create such note without the field being set.

roman-r-m · 2021-06-06T21:20:40Z

This field being set is what defines a conflicted note, so it would definitely be set. When a conflict is detected a new note is created with "is_conflict" set to 1. So it's not possible to create such note without the field being set.

I meant the new field that this PR introduces -- conflict_parent_id

… folder.

Ahmad45123 · 2021-06-06T21:39:04Z

I was trying to say that when @Ahmad45123 is going to add logic that relies on this field he can't assume that each conflicted note will have it set.

Oh right! Ty for the note. Though, I think that will be handled in the plugins side? Maybe I'll use it if exists and otherwise ask the user to select the other note as some sort of backwards compatibly.

I've also made it to reset conflict_parent_id when copying or moving notes. I think that's the only case where is_conflict is modified to zero from one.

packages/lib/services/synchronizer/Synchronizer.conflicts.test.ts

packages/lib/models/Note.ts

Ahmad45123 · 2021-06-08T21:44:09Z

Ok so I've done:

Added tests for copyToFolder and moveToFolder. This is like my first ever unit test so please excuse me if there's any issues.
I also did what you suggested in Synchronizer.conflicts.test.ts

packages/lib/models/Note.test.ts

Ahmad45123 · 2021-06-09T19:36:14Z

I've fixed the issues. I hope it's okay now :)

laurent22 · 2021-06-09T19:58:34Z

packages/lib/models/Note.test.ts

+		const tmpConflictedNote = Object.assign({}, note1);
+		delete tmpConflictedNote.id;
+		tmpConflictedNote.is_conflict = 1;
+		tmpConflictedNote.conflict_original_id = note1.id;
+		const conflictedNote = await Note.save(tmpConflictedNote, { autoTimestamp: false });


We don't copy and paste large blocks of code in this codebase, it's just a mess to maintain otherwise. So please do this:

Create a function in models/Note called public async createConflictNote(sourceNote:NoteEntity, changeSource:number):Promise<NoteEntity>

Make it create the conflict note and save it as in the code above.

Then use that function in:

Your two test units

In lib/Synchronizer.ts

As a general rule if you see you are copying more than two or three lines of code, you should stop and think if the code can be refactored. Such refactoring improves the code quality because it wraps and names (using the function name) business logic and makes it re-usable.

Should be good now.

Ahmad45123 · 2021-06-10T23:01:04Z

This is ready for another review.

laurent22 · 2021-06-10T23:18:33Z

packages/lib/models/Note.test.ts

+		// Source conflict should stay as is
+		expect(conflictedNote.is_conflict).toBe(1);
+		expect(conflictedNote.conflict_original_id).toBe(note1.id);
+		expect(conflictedNote.parent_id).toBe(srcfolder.id);


If you want to test the output of createConflictNote, it should be in a separate test, so please remove this part.

I'll move it to a separate test.

laurent22 · 2021-06-10T23:20:11Z

packages/lib/models/Note.test.ts

+
+		const conflictedNote = await Note.createConflictNote(note1, ItemChange.SOURCE_SYNC);
+
+		// Move


In your comments you should explain why, not what. We know that a function called "Note.moveToFolder" is going to move the note to a folder, and a comment for this is just noise. I don't think you need to explain anything here, so please remove the comment.

laurent22 · 2021-06-10T23:21:07Z

packages/lib/models/Note.test.ts

+
+		const conflictedNote = await Note.createConflictNote(note1, ItemChange.SOURCE_SYNC);
+
+		// COPY File


Your comment explains nothing and is even confusing since we aren't moving files but notes, so please remove it.

Noted. Also no idea myself where did "File" come from 😂 sorry about that

laurent22 · 2021-06-10T23:21:21Z

packages/lib/models/Note.test.ts

@@ -332,4 +333,49 @@ describe('models_Note', function() {
 		expect(sortedNotes3[4].id).toBe(note2.id);
 	}));

+	it('should copy conflicted note to target folder and cancel conflict', (async () => {
+		// Prepare


laurent22 · 2021-06-10T23:21:29Z

packages/lib/models/Note.test.ts

+	}));
+
+	it('should move conflicted note to target folder and cancel conflict', (async () => {
+		// Prepare


…g and copying notes.

Ahmad45123 · 2021-06-11T09:58:18Z

Should be good for another review. :)

laurent22 · 2021-06-11T10:05:12Z

Ok, you're the first pull request that uses the new GitHub Actions CI, so let's see if all the tests pass.

Ahmad45123 · 2021-06-11T10:41:35Z

Hmm I'll check the failed tests in a few minutes.

laurent22 · 2021-06-11T12:11:37Z

If you are going to fix the test by committing and waiting for the full CI to run it might take a while. Instead you should look where the failure happens and run the tests locally inside the right package only.

In that particular case, you can run npm test from within packages/server.

Ahmad45123 · 2021-06-11T12:13:39Z

If you are going to fix the test by committing and wait for the full CI to run it might take a while. Instead you should look where the failure happen and run the tests locally inside the right package only.

In that particular case, you can run npm test from within packages/server.

I tried but I'm having trouble getting the server tests running locally. In fact, thats exactly why I missed this failing test. They all fail so I just ignored them.

This is a portion of logs: https://pastebin.com/YXpyKNwx

I think it has to do with Sqlite somehow.

laurent22 · 2021-06-11T12:14:55Z

Could you use WSL instead? Otherwise you'll keep getting issues like this.

Ahmad45123 · 2021-06-11T23:40:01Z

Spent most of the day to get WSL working haha.. And now all tests pass locally though I've got no idea why did CI fail again :/
Seems you made changes to it though ? Doesn't even reach the actual tests stage if I make sense.

E: Please review my fix to that server test btw, I don't know how that all works yet and I've got no idea if my fix is correct haha

laurent22 · 2021-06-12T07:46:34Z

There was a problem with the CI config but it works now, so let's merge.

Ahmad45123 and others added 3 commits June 3, 2021 21:13

Added new column conflict_parent which stores the note_id of the othe…

861f69a

…r note in conflict.

Merge branch 'laurent22:dev' into dev

763a822

Fixed tests for conflicts.

a597284

Ahmad45123 added 3 commits June 6, 2021 23:30

Changed name of the column to conflict_parent_id

8b3a1d9

Rename in other files too

3c9097e

Reset conflict_parent_id when moving or copying notes out of conflict…

3819ddc

… folder.

laurent22 reviewed Jun 6, 2021

View reviewed changes

packages/lib/services/synchronizer/Synchronizer.conflicts.test.ts Outdated Show resolved Hide resolved

packages/lib/models/Note.ts Outdated Show resolved Hide resolved

packages/lib/models/Note.ts Outdated Show resolved Hide resolved

Ahmad45123 added 3 commits June 8, 2021 22:40

Added new column to types.ts

b293b80

Added tests to copyNote and moveNote

07158ff

Fixed Sync conflicts tests

322f11c

laurent22 reviewed Jun 8, 2021

View reviewed changes

packages/lib/models/Note.test.ts Outdated Show resolved Hide resolved

packages/lib/models/Note.test.ts Outdated Show resolved Hide resolved

packages/lib/models/Note.test.ts Outdated Show resolved Hide resolved

packages/lib/models/Note.test.ts Outdated Show resolved Hide resolved

Ahmad45123 added 2 commits June 9, 2021 21:01

Renamed the column from conflict_parent_id to conflict_original_id

555a4b7

Rewrote tests for the conflict_original_id column

7599fb2

Ahmad45123 changed the title ~~All: Conflict notes will now populate a new field conflict_parent with the id of the conflict note,.~~ All: Conflict notes will now populate a new field conflict_parent with the id of the conflict note. Jun 9, 2021

laurent22 reviewed Jun 9, 2021

View reviewed changes

Changed to use a new function createConflictNote.

85ba6c5

laurent22 reviewed Jun 10, 2021

View reviewed changes

Added new test for createConflictNote and cleaned the tests for movin…

21811c8

…g and copying notes.

Ahmad45123 and others added 2 commits June 11, 2021 13:32

Merge branch 'laurent22:dev' into dev

1b8fdca

Attempt at fixing the server error related to conflict_original_id

c162d50

Fixed test failing in server related to new original_id column.

06ab8ed

Update testUtils.ts

3b99bab

laurent22 merged commit 2af3bf6 into laurent22:dev Jun 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

All: Conflict notes will now populate a new field conflict_parent with the id of the conflict note. #5049

All: Conflict notes will now populate a new field conflict_parent with the id of the conflict note. #5049

Ahmad45123 commented Jun 3, 2021

laurent22 commented Jun 5, 2021

roman-r-m commented Jun 5, 2021 •

edited

Loading

Ahmad45123 commented Jun 5, 2021 •

edited

Loading

laurent22 commented Jun 6, 2021

roman-r-m commented Jun 6, 2021 •

edited

Loading

laurent22 commented Jun 6, 2021

roman-r-m commented Jun 6, 2021

Ahmad45123 commented Jun 6, 2021 •

edited

Loading

Ahmad45123 commented Jun 8, 2021

Ahmad45123 commented Jun 9, 2021

laurent22 Jun 9, 2021 •

edited

Loading

Ahmad45123 Jun 10, 2021

Ahmad45123 commented Jun 10, 2021

laurent22 Jun 10, 2021

Ahmad45123 Jun 10, 2021

laurent22 Jun 10, 2021

laurent22 Jun 10, 2021

Ahmad45123 Jun 10, 2021

laurent22 Jun 10, 2021

laurent22 Jun 10, 2021

Ahmad45123 commented Jun 11, 2021

laurent22 commented Jun 11, 2021

Ahmad45123 commented Jun 11, 2021

laurent22 commented Jun 11, 2021 •

edited

Loading

Ahmad45123 commented Jun 11, 2021 •

edited

Loading

laurent22 commented Jun 11, 2021

Ahmad45123 commented Jun 11, 2021 •

edited

Loading

laurent22 commented Jun 12, 2021


		const conflictedNote = await Note.createConflictNote(note1, ItemChange.SOURCE_SYNC);

		// Move


		const conflictedNote = await Note.createConflictNote(note1, ItemChange.SOURCE_SYNC);

		// COPY File

All: Conflict notes will now populate a new field conflict_parent with the id of the conflict note. #5049

All: Conflict notes will now populate a new field conflict_parent with the id of the conflict note. #5049

Conversation

Ahmad45123 commented Jun 3, 2021

laurent22 commented Jun 5, 2021

roman-r-m commented Jun 5, 2021 • edited Loading

Ahmad45123 commented Jun 5, 2021 • edited Loading

laurent22 commented Jun 6, 2021

roman-r-m commented Jun 6, 2021 • edited Loading

laurent22 commented Jun 6, 2021

roman-r-m commented Jun 6, 2021

Ahmad45123 commented Jun 6, 2021 • edited Loading

Ahmad45123 commented Jun 8, 2021

Ahmad45123 commented Jun 9, 2021

laurent22 Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ahmad45123 commented Jun 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ahmad45123 commented Jun 11, 2021

laurent22 commented Jun 11, 2021

Ahmad45123 commented Jun 11, 2021

laurent22 commented Jun 11, 2021 • edited Loading

Ahmad45123 commented Jun 11, 2021 • edited Loading

laurent22 commented Jun 11, 2021

Ahmad45123 commented Jun 11, 2021 • edited Loading

laurent22 commented Jun 12, 2021

roman-r-m commented Jun 5, 2021 •

edited

Loading

Ahmad45123 commented Jun 5, 2021 •

edited

Loading

roman-r-m commented Jun 6, 2021 •

edited

Loading

Ahmad45123 commented Jun 6, 2021 •

edited

Loading

laurent22 Jun 9, 2021 •

edited

Loading

laurent22 commented Jun 11, 2021 •

edited

Loading

Ahmad45123 commented Jun 11, 2021 •

edited

Loading

Ahmad45123 commented Jun 11, 2021 •

edited

Loading